fix(deps): preflight runtime dependencies - #2557

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 05f48a5ee2

Code review for commit 35746f1 — approving.

What I verified

• Ran tests/test-build-all-runtime-deps.sh locally against this commit: 28/28 pass.
• Ran runtime_dependencies_for_manifest against all 8 real component manifests: emits exactly the expected 15 records; agent-memory and os-skills correctly contribute zero.
• Preflight ordering confirmed in code and tests: platform capabilities are checked before user source setup; system mode blocks platform-capability / language-runtime blockers before any package mutation; one deduplicated APT transaction preceded by apt-get update (failure stops before mutation); re-probe after install; install phase re-verifies before any component file lands.
• The two earlier automated findings (sec-core systemd exclusion for source builds, APT refresh before the aggregated install) are both addressed in this commit with regression tests.
• User mode never calls as_root for runtime packages; system probes use the fixed RUNTIME_SYSTEM_PATH, so a user-local nvm Node cannot satisfy the system contract; the emitted retry command is absolute and preserves --system / --deps-only / --ignore-deps (covered for --component memory).
• agent-memory code is untouched; it participates via the generic manifest-driven preflight (it declares no dependencies), so no new requirements are introduced for memory installs.
• The ws-ckpt probe change (btrfs version → mkfs.btrfs --version) matches what the removed installer actually required (cmd_exists mkfs.btrfs).

Non-blocking suggestions

1. scripts/build-all.sh:2128,2150 — the local -n namerefs are the script's first bash ≥ 4.3 dependency. On bash 4.2 hosts (CentOS/Anolis 7 era) the preflight would error out instead of reporting. Likely acceptable given the ws-ckpt/sight kernel floors and EL7 EOL, but worth confirming EL7 is out of support scope, or documenting a minimum bash version.
2. User-visible behavior change worth a release note: missing runtime dependencies now hard-fail user-mode installs, where bwrap/gpg/jq previously only warned. Intentional per the PR body, but users will notice.
3. The awk TOML reader assumes single-line packages = { ... } inline tables and no | character in any field. Current manifests comply and the 15-record assertion guards against drift, but consider a one-line comment near runtime_dependencies_for_manifest spelling out these constraints so future manifest edits don't silently drop records.

Nice work on the aggregated reporting, the strict order-of-operations guarantees, and the test coverage.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35746f1d87

Code review for commit a463f7bf (re-review after force-push squash) — approving.

What I verified

P1 fix (skip runtime checks for user-ineligible components). The delta vs the previous head 35746f1d is exactly the fix plus one regression test: a new runtime_install_components() filter (user mode drops ws-ckpt) now feeds both selected_runtime_dependencies and source_build_runtime_dependencies. I confirmed this is correct:

• src/ws-ckpt/Makefile makes INSTALL_PROFILE=user install a genuine no-op ("User mode: ws-ckpt requires a systemd/root daemon; skipping install."), and its manifest declares [component.layout] modes = ["system"], so its daemon runtime/btrfs requirements are irrelevant in user mode.
• Ran the real (non-stubbed) collection logic on a simulated btrfs-less host (empty /proc/filesystems entry + empty RUNTIME_SYSTEM_PATH so modprobe is unavailable): user mode with default components collects 0 platform-capability gaps (previously aborted before any dependency setup), while system mode with --component ws-ckpt still correctly reports ws-ckpt|btrfs [platform-capability] as missing.
• System mode with default components retains all three ws-ckpt records (btrfs-progs with the mkfs.btrfs --version probe, rsync, btrfs platform capability).
• The filter is correctly scoped: sight is the only other mode-restricted component, but its user profile genuinely installs files (user PREFIX, INSTALL_SYSTEMD=0), so it rightly remains unfiltered.

Tests. Ran tests/test-build-all-runtime-deps.sh against this commit locally: 29/29 pass, including the new test_user_skips_ws_ckpt_noop_install_dependencies covering both directions (user default omits ws-ckpt; explicit system ws-ckpt keeps it).

Earlier P2s remain satisfied in the squashed diff. sec-core systemd is excluded from source-build dependencies (runtime_dependency_for_source_build), and DEB system preflight refreshes APT indexes before the single deduplicated transaction with a hard stop on refresh failure. Both retain their regression assertions.

Full squashed single-commit diff reviewed (all 5 files, +1300/-55): manifest-driven awk parser with key-based (not order-based) reads, source-build adapters (openssl1.1→openssl, sec-core nodejs→node >=20), two-phase user preflight (platform-only → source setup → full), system preflight blocking platform/language-runtime blockers before any package mutation, fixed RUNTIME_SYSTEM_PATH contract for system installs (nvm can't satisfy it), absolute retry commands preserving --system/--deps-only/--ignore-deps, and the guarded main enabling the test harness to source the script. CI changes are consistent: the sec-core source-build jobs preinstall the runtime contract (or let system-mode preflight install it on a clean apt state, which exercises the new aggregated path), and the new check-build-runtime-deps job wires the test suite into CI. agent-memory participates via its manifest, which declares no dependencies — no new requirements for memory installs.

Non-blocking notes (no action required)

1. The user-mode ws-ckpt exemption is hardcoded; if more mode-restricted components appear, deriving the filter from [component.layout] modes would generalize it. Current approach is explicit, commented, and test-covered, so this is fine for now.
2. Carry-over from the previous review, still present and still acceptable: local -n namerefs require bash >= 4.3, and missing system packages now hard-fail user-mode installs (previously warn-only). Both were already acknowledged.

Code review 通过（re-review @ 70d6507）。

对比上一轮已批准的 head a463f7b，70d6507 的变更仅限 .github/workflows/sec-core-source-code-build.yaml（+17/-2），两处 CI 改进均正确：

1. UV_PYTHON_INSTALL_MIRROR 指向官方 astral-sh python-build-standalone 源：scripts/build-all.sh 中 _configure_uv_mirror 使用 ${UV_PYTHON_INSTALL_MIRROR:-<nju mirror>}，workflow 级 env 在 CI 中正确生效，本地构建默认值不受影响，与 PR body 描述一致。
2. 构建失败时打印 target/build.log 最后 200 行（::group:: 折叠）并保留原始退出码：日志路径与 LOG_FILE="$OUTPUT_DIR/build.log"（OUTPUT_DIR=$PROJECT_ROOT/target）一致；在 GitHub Actions 默认 bash -e 下逐句验证（|| build_status=$? 捕获、if (( ... )) 条件上下文、tail ... || echo 兜底、exit "$build_status"），成功/失败两条路径行为均正确，用户模式成功后追加 $GITHUB_PATH 的顺序也保留。

同时确认：

• scripts/build-all.sh、manifests、tests 在两个 head 之间无任何变更，上轮验证过的逻辑原样保留；bot 的 P1 修复（user 模式跳过不安装文件的 ws-ckpt 的 runtime 依赖收集）在最终 head 仍成立。
• 本地回归 ./tests/test-build-all-runtime-deps.sh 在 70d6507 上 29/29 通过；YAML 解析与 bash -n 均通过。
• CI（70d6507）：Source Build (Ubuntu 22.04 / Alinux4)、Source Build System Install、Check build runtime dependencies、Test anolisa 等全部 SUCCESS，无 failure。

代码质量良好，未发现新问题，approve。

Thanks for the well-engineered preflight rewrite — the aggregated manifest-driven probing, user/system mode split, fail-closed version checks, and the 29-case test suite were all verified locally (29/29 shell tests, 93 anolisa-core manifest tests reproduced). A few contract regressions need attention before merge:

P1 — Must Fix

1. --ignore-deps contract regression: the flag can no longer skip runtime dependency enforcement, and help/docs were not updated

• do_install() now unconditionally runs preflight_runtime_dependencies || return 1 (L2465-L2472) with no $INSTALL_DEPS gate.
• Inside the preflight, the [[ "$INSTALL_MODE" != "system" ]] || ! $INSTALL_DEPS branch (L2270-L2274) reports and returns 1 for user mode or --ignore-deps.
• Baseline behavior: --ignore-deps = "build + install, skip dep install" (help L8/L2776/L2795/L2804, AGENTS.md, docs/BUILDING_zh.md), and missing bwrap/gpg/jq in user mode was warn-only.

Net effect: ./scripts/build-all.sh --ignore-deps --component sec-core (the exact usage documented in the help at L2804) on a minimal container without jq/bwrap now completes the entire build first, then fails at install time — and no flag remains to bypass the runtime dependency check (--no-install doesn't install at all). The PR body states "No documentation changes are required", but the flag semantics are inverted relative to the help/docs.

Please either gate the preflight on $INSTALL_DEPS and downgrade to an aggregated warning under --ignore-deps, or keep the strict behavior but update the help (L8/L2776/L2795/L2804), AGENTS.md, and BUILDING*.md to state that runtime dependencies are still verified and must be present.

P2 — Should Fix

2. System-mode Node repository-install path is now unreachable dead code; documented --system usage starts failing on hosts it used to work on

• do_install_deps() runs the full system-mode preflight before install_node (L1458-L1464); sec-core's nodejs dependency is rewritten as a language-runtime >= 20 blocker, so a host without Node aborts immediately.
• install_node()'s repository-install branch and its die "Repository Node.js cannot satisfy ..." (L655-L662) can therefore never execute in the normal flow — only the unit tests reach it.
• A host whose distro repo provides Node >= 20 but where Node isn't installed yet (previously: dnf install nodejs npm → success) now fails per the documented --system usage (docs/BUILDING_zh.md). CI compensates by preinstalling Node 24 + symlink; local users cannot.

Consider letting the system-mode preflight attempt install_node's repository path once before blocking, and document the Node >= 20 precondition for --system in BUILDING*.md / QUICKSTART.

3. User mode tightened from warn-and-continue to hard failure without doc updates

Missing bubblewrap/gnupg/jq in user mode now aborts (L1494-L1497 + L2465-L2470) instead of warn-only. The fail-fast intent is sound, but the QUICKSTART path (./scripts/build-all.sh --component sec-core, docs/user-guide//agent-sec-core/QUICKSTART.md) and BUILDING.md should list the runtime prerequisites (bubblewrap, gnupg, jq, python3, pkg-config + openssl-devel), or provide a --skip-runtime-check escape hatch (could share the mechanism with item 1).

4. Self-hosted runner global state: /usr/local/bin/node symlink persists across jobs

Both jobs in .github/workflows/sec-core-source-code-build.yaml run sudo ln -sfn "$(command -v node)" /usr/local/bin/node (L52-L55, L133-L136) with no cleanup. If the anolisa-k8s-general-ci-x64 pool reuses pods, the symlink leaks into other workflows' assumptions about the system PATH, and future setup-node version changes silently rewrite it. Please add an if: always() cleanup step (sudo rm -f /usr/local/bin/node), or use a side-effect-free probe that expands $(command -v node) directly.

P3 — Minor (non-blocking)

• The shell TOML parser only supports single-line inline tables (packages = { rpm = ..., deb = ... }); a future multi-line inline table or dotted key would silently yield empty package names while the Rust side parses fine — consider warning on an unclosed packages = {.
• Unparseable language-runtime version: the shell side judges "missing" (L2058-L2063) while anolisa-core's version_verdict treats it as NotVerified → Resolved — verdicts would diverge if a non-semver runtime is ever added.
• User-mode default flow runs the full preflight twice (do_install_deps tail L1495-L1497 and do_install L2469-L2470) — log noise only.
• UV_PYTHON_INSTALL_MIRROR switched to GitHub direct while the rest of this pipeline uses China mirrors — worth watching this workflow's stability.
• nvm fallback bumped to Node 24 while build-copilot-shell CI pins Node 20 — matrix divergence for a native-module-dependent package.
• local -n raises the implicit bash requirement to >= 4.3 (fine for supported distros, but undeclared).
• anolisa-core/src/manifest.rs doc comment still shows the old btrfs version probe example (the ws-ckpt probe change itself is verified safe for all downstream consumers).
• Cosmetics: trailing spaces in the retry/install hint lines (printf '%q '); command -v gpg in the CI verify steps prints the path into logs (add >/dev/null).

Summary: completeness and implementation quality are excellent — all 10 "What changed" claims verified in code, and the manifest key contract is consistent between build-all.sh and anolisa-core. The blocker is the --ignore-deps semantics inversion plus the undocumented tightening of user/system mode failure behavior; the rest are documentation and CI hygiene items.

Thanks for the thorough response — all four blocking findings from the previous round are verified as resolved (with tests and docs): the --ignore-deps bypass is fully restored end-to-end (preflight skipped, install proceeds), the system-mode Node requirement is documented with an actionable defensive die, the user-mode prerequisites are documented accurately against the actual probe set, and the CI node symlink is now backed up and restored on both success and failure. The re-run test suite (34/34) and workflow YAML validation were reproduced locally. One documentation gap remains before merge, plus a few non-blocking notes:

P2 — Should Fix

1. uv Python download source flipped to GitHub direct for all local builds, but the UV_PYTHON_INSTALL_MIRROR escape hatch is documented nowhere

_configure_uv_mirror() now defaults python-install-mirror to https://github.com/astral-sh/python-build-standalone/releases/download (previously the NJU mirror, which existed for restricted networks) and migrates build-all-managed legacy configs in place. However, a repo-wide search finds zero mentions of UV_PYTHON_INSTALL_MIRROR / python-install-mirror in docs/, READMEs, CONTRIBUTING, the website, --help, or usage() — users behind restricted networks (the original audience of the NJU default; the rest of the pipeline still defaults to aliyun PyPI / npmmirror) whose Python download now fails or stalls have no documented recovery path.

Suggested fix (~4 lines, bilingual, in the BUILDING.md / BUILDING_zh.md prerequisites section):

Python runtimes are downloaded from GitHub (astral/python-build-standalone) by
default; on restricted networks set `UV_PYTHON_INSTALL_MIRROR` to a reachable
mirror before building.

P3 — Non-blocking

• --help (L2804) and AGENTS.md (L32) still describe --ignore-deps as "Skip dependency installation" only; the flag now also skips runtime dependency verification (documented in BUILDING.md, but --help is the first contract surface users see). A one-line update would close the gap: --ignore-deps Skip dependency installation and runtime dependency verification (pre-provisioned hosts only).
• The CI expose step's trap restore_node ERR does not cover hard cancellation (SIGTERM) in the sub-second window between the sudo mv backup and the $GITHUB_ENV write, where the symlink could leak. Well-mitigated as is (if: always() restore runs on cancel, per-job pods, self-healing fixed-name backup), so purely optional hardening: trap restore_node ERR EXIT.
• Remaining minor items from the previous round, acceptable to defer: unparseable-version semantics divergence between the shell preflight (missing) and anolisa-core's version_verdict (NotVerified → Resolved); single-line-only inline-table support in the shell TOML parser; stale btrfs version probe example in anolisa-core/src/manifest.rs doc comments; nvm Node 24 vs copilot-shell CI Node 20 matrix divergence; duplicate user-mode preflight log noise.

Everything else in the increment checks out: the do_install --ignore-deps gate restores the bypass semantics exactly (preflight call count 0, install proceeds, dry-run branch symmetric), the install_node die relocation leaves no reachable system-mode repository-install path, the uv legacy migration is marker-gated and fail-open (user-owned configs untouched), and the CI backup/restore handles dangling symlinks, in-step ERR, and cancellation. Happy to approve once the P2 doc addition lands.

All findings from the previous rounds are now resolved and verified — thanks for the thorough iterations:

• The --ignore-deps bypass semantics are fully restored and consistently documented across --help, AGENTS.md, BUILDING, QUICKSTART, and the sec-core READMEs.
• The UV_PYTHON_INSTALL_MIRROR escape hatch is now documented bilingually in BUILDING.md / BUILDING_zh.md (§3 toolchain section) with an example, matching the actual _configure_uv_mirror behavior point by point.
• The CI node symlink backup/restore, the system-mode Node >= 20 requirement, and the Bash 4.3 minimum are all in place; the test suite (34/34) was reproduced locally across rounds, and the final force-push is a clean text-only increment (+22/-5) with no hidden logic changes.

One last non-blocking suggestion: CONTRIBUTING.md (L128) and CONTRIBUTING_zh.md (L106) still describe --ignore-deps as "Skip dependency installation / 跳过依赖安装" — the only two stale references left repo-wide after this PR's semantic change. A two-line follow-up (or a quick amend if it fits this PR's scope) would make the flag's documentation fully consistent:

| `--ignore-deps` | Skip dependency setup and runtime verification (pre-provisioned hosts only). |

| `--ignore-deps` | 跳过依赖安装与运行时校验（仅适用于已预装依赖的主机）。 |

Approving — great work on this PR. The preflight design (aggregated manifest-driven probing, user/system mode split, fail-closed version checks, marker-gated legacy migration) and the responsiveness across review rounds are both excellent.